说说 Code Review
题图:frometta.livejournal.com
之前写过一篇《写代码的四个境界》,那个时候,大部分时候我还是愉快地写着自己的代码。Code review 也是每天工作的一部分,但是相对而言花的时间还是有限的。
最近一是因为角色转换,二是突然来了很多新人。花在 code review 上的时间比写代码多出了好多,也有一些心得和感触,随便写写吧。
总的说来,硅谷稍具规模的公司 code review 的流程都是比较规范的。模式也差不多。一来所有的 PR 都必须有至少一个人 stamp,才能 merge。如果改的东西涉及到多个项目,则需要每个项目都有人 stamp 才行。还有一些特别关键的代码,比如支付相关的,通常也会需要支付组的人 stamp 才行。
NOTE: PR (pull request, a set of changes someone tries to push to a GitHub repository. Once a PR is sent, interested parties (peer engineers) can review the set of changes, discuss potential modifications, and even push follow-up commits if necessary.
在 stamp 前,通常是 github page 上可以给出各种 comments,page 是共享的,所以谁都能看到。有些 comments 是询问,那么代码作者直接回复或解释就行。有些 comments 是指出代码的问题,这样代码作者可能会依此改动;也可能不同意,那就回复 comments,有的时候关于一个实现细节,讨论的 comments thread 可以多达十几条。这样对于大家达成共识很有帮助。
以前遇到有朋友说:
“看到代码写的太烂,觉得来回扯皮效率不高,干脆扑上去自己写。”
曾经我也有过类似的想法。不过最近遇到的一些事让我慢慢地意识到这种想法很要不得。
首先就是从对方的角度来说。对方写不好,可能是对业务不熟悉,可能是对语言不熟悉,也可能是对公司代码的大架构不熟悉。如果你是帮他/她 “写” 而不是耐心指出哪里有问题,那么下一次,他/她可能还是不知道。不仅无益于别的人成长,有的时候甚至会让别人有挫败感。
再然后就是这种方式的可扩展性很差。即使 code review 会花掉哪怕十倍于你自己写的时间和精力,但它会让人明白代码应该怎么写,从长远来看,这其实是在一定程度上 “复制” 你的生产力。你不能什么都自己写。尤其是你慢慢开始带项目、带新人。每天 review 五个人的代码,和写五个人的代码,你觉得长期而言哪个更合算?
此外,如果说写代码是一个学习过程,怎么做一个好的代码审核人更是一个学习和成长的过程。自己绕过一个坑不难,难的是看到别人那么走,远远地你就能告诉他/她那里有个坑。而他/她在经你指出多次后,下一次他/她也会帮着指出别人的类似的问题。
这一点最近感触尤为深刻。前一阵组里咔咔咔接二连三来了好多新人,老大说,你少写点代码,多做点 review。于是那几天我几乎工作的一半时间都用来看代码,写 comments。可是最近就会发现,他/她们相互之间大部分时候已经可以很好的相互 review 了,于是我又腾出好一些时间来做别的工作。
Code review 做多了,发现其实也是有一定套路的,试着归纳如下:
代码格式方面。很多公司都有 coding style guideline。大家的约定俗成,避免公司的代码风格不一致,也避免一些不不要的为了 “要不要把闭括号另起一行” 而无谓地争论,除非是不小心,通常大家都不会弄错。但是新员工往往会在这方面还不太熟悉。这一类问题也比较容易指出。
代码可读性方面。这包括一个函数不要太长,太长就 break down。所有的变量名尽量能够说明它的用意和类型。比如 hosting_address_hash,一看就知道是房东地址,而且是个 hash 类型。不要有嵌套太多层的条件语句或者循环语句。不要有一个太长的 boolean 判断语句。如果一个函数,别人需要看你的长篇注释才能明白,那这个函数就一定有重构的空间。另外,如果不可避免有一些注释,则一定要保证注释准确且与代码完全一致。
帮代码作者想想他/她有没有漏掉任何 corner case。很多时候这是业务逻辑相关的,尤其需要比较老的工程师帮助指出需要处理的所有情况。
Error handling。这是最常见也是代码审核最容易帮别人看出的问题。举个例子,下面一段简单到不能再简单的代码就至少有三个潜在的问题:params 里面需要 validate 是不是有 user_id 和 new_name 这两个 key;能不能找到这个 user_id 对应的 user;save 的时候会不会有 DB level 的 exception,应该怎么处理。
测试例和防坑。测试例不用说了,每段代码都应该有测试例。但是对于一些你能预见如果别人改动代码会引起可能问题的情况,一定要额外的加测试例防止这种事情的发生。这一点没有例子参考也不太好说。怎么写好测试例,本身就值得写一篇文章了。
小架构。什么意思呢,就是一个文件或者类内部的代码组织。比如如果有重复的代码段,就应该提取出来公用。不要在代码里随意设常数,所有的常数都应该文件顶部统一定义。哪些应该是 private,等等
大架构。这个就更广了。包括文件组织,函数是不是应该抽象到 lib 或者 helper 文件里;是不是应该使用继承类;是不是和整个代码库的风格一致,等等。
当然,视具体情况可能还有很多别的需要在 code review 中注意的。但是如果按照这个 checklist 过一遍,一些明显的问题就可以避免个八九不离十了。
另外,代码 review 和写代码是我们每个工程师都应该致力的两个方面,缺一不可。可能在工作中的某个阶段或者某个项目里,你会花更多的时间在某一面,但长久看来,两个技能缺一不可。
代码审核是工作,不要抱有情绪化。我曾经干过一件事,一个外组同事的代码,别人已经 stamp 了,可是我觉得有问题,于是把 stamp revert 掉了,在 comments 里写了为什么有问题,和建议的改法。当时心里还有点觉得好像怪得罪人的。可是后来发现同事反而很客气地接受并道谢了。心里也是很感激有这样的工作环境。
另外,经常会遇到有些 review 的 comments 里出现不同意见,其实是两个人在编程习惯和约定俗成上没有共识。如果在不违背公司 style guideline 的情况下,没必要一定让对方和你有一样的习惯。如果你真的觉得这样更好,通常我也只会写 “I personally would prefer A over B, but no strong opinion.” 或者 “How about change it to A, since ... However, this is not a merge blocker. ”
人都有不周到的时候。多几双眼睛一起帮你看一遍,就可以很大程度地减少代码里的错误。另外,相互 review 的过程中还能从彼此那里学到很多编程的小技巧和好习惯。细想来,很多 Java 和 Ruby 的特别好用的 feature,我都是在 code review 的过程中学到的。